refactor: simplify terminal.text.width using luasystem 0.7.0 (#204) by Gitkbc · Pull Request #206 · lunarmodules/terminal.lua

Gitkbc · 2026-02-24T05:54:52Z

Implements the simplification proposed in #204.

Introduce a single ambiguous-width calibration during
terminal.initialize() (and preload_widths).
Remove per-character width caching and runtime probing logic.
Delegate width calculations to LuaSystem.
Preserve test and test_write APIs for compatibility.
Bump dependency to luasystem >= 0.7.0.

Fixes #204.

Remove per-character width cache and probe a single ambiguous-width character during initialization. Store the measured value globally and delegate width calculation to LuaSystem. Preserve test and test_write APIs for compatibility. Update documentation and rockspec dependency.

Tieske

@Gitkbc I assume you created this PR as an intro into GSoC.

Seems like you had an AI friend create this PR. My suggestion would be to try and understand the issue at hand first, and then try again by writing the code yourself.

Tieske · 2026-02-24T18:55:03Z

src/terminal/text/width.lua

-  -- re-run to get the total width, since all widths are known now,
-  -- but this time do not write the string, just return the width
-  return M.test(str)
+  M.initialize()


I don't think there is any utility left for this function. It was an optimization of the test function, in case you would also want the text to actually show.

Tieske · 2026-02-24T18:56:34Z

src/terminal/text/width.lua

-- character not in the cache will be passed to `system.utf8cwidth` to determine the width.
+--- Character display width helpers.
+-- Uses LuaSystem width calculations with an optional calibrated
+-- ambiguous-width value for terminal-specific behavior.


can we leave some more background in the doc comments? the cache part should go, that's clear, but the ambiguous explanation is still useful I think

Tieske · 2026-02-24T18:57:10Z

src/terminal/draw/init.lua



-- returns a string with all box_fmt characters, to pre-load the character width cache
+-- returns a string with all box_fmt characters


this has no utility anymore, it was only here to be able to test the default used characters

Tieske · 2026-02-24T18:59:54Z

src/terminal/init.lua

 -- @within Initialization
 function M.preload_widths(str)
-  text.width.test((str or "") .. M.progress._spinner_fmt_chars() .. M.draw._box_fmt_chars())
+  if str then


this function also has no utility anymore

Tieske · 2026-02-24T19:01:42Z

src/terminal/init.lua

      sys.setconsoleflags(io.stdin, sys.getconsoleflags(io.stdin) - sys.CIF_PROCESSED_INPUT)
    end

+    text.width.initialize(true)


testing should be optional. The default can be to test, but the user should be able to skip it. In case of a pipe-out, instead of a terminal, write the test characters would break the output.

Tieske · 2026-02-24T19:02:40Z

src/terminal/text/width.lua

+local ambiguous_char = "·"
+local ambiguous_codepoint = utf8.codepoint(ambiguous_char)
+
+M.ambiguous_width = nil


I think it is save to set a default here and document it. Default should be 1.

Tieske · 2026-02-24T19:03:56Z

src/terminal/text/width.lua

+
+
+local function detect_ambiguous_width()
+  local row, col = t.cursor.position.get()


should we use istty() here to see if we have a terminal?

Tieske · 2026-02-24T19:05:07Z

src/terminal/text/width.lua

+  local query = ambiguous_char .. t.cursor.position.query_seq() .. setpos
+
+  t.text.stack.push({ brightness = 0 })
+  local result, err = t.input.query(query, "^\27%[(%d+);(%d+)R$")


don't hardcode the sequence, use this library's functions

Gitkbc · 2026-02-24T20:38:36Z

Thanks for the detailed review and the clear pointers, @Tieske.

You're right — the initial submission didn’t fully align with the project’s design and conventions. I’ll rework the implementation to address each of your points and ensure it matches the existing architecture and standards.

I’ll push a revised version shortly.

Appreciate the thorough feedback.

… calibration (LuaSystem >= 0.7.0)

Gitkbc · 2026-02-26T20:51:12Z

Addressed the requested changes:
1)Simplified terminal.text.width to delegate to LuaSystem (>= 0.7.0)
2)Removed width caching and preload mechanisms
3)Introduced optional ambiguous-width calibration during initialize()
4)Restored terminal.utils export to preserve the public API
5)All specs passing locally (851 tests, 0 failures).

Please let me know if any further adjustments are needed.

Tieske

Please review your own PR before submitting, there's quite a few things in here that were obviously wrong.

Tieske · 2026-02-27T11:14:58Z

src/terminal/draw/line.lua

+  local stripped_title = utils.strip_ansi(title)
+  local stripped_title_w = text.width.utf8swidth(stripped_title)
+  local title_w
+  if stripped_title_w <= w_for_title then
+    title_w = stripped_title_w
+  else
+    stripped_title, title_w = utils.truncate_ellipsis(w_for_title, stripped_title, type)
+    title = stripped_title
+  end
+


shouldn't most of this logic actually go into the truncate_ellipsis function?

Tieske · 2026-02-27T11:16:30Z

src/terminal/draw/line.lua



-return M
+return M


nitpick; this is an unwanted whitespace change. you can configure you edito probably to honor the settings in .editorconfig to ensure it automatically adheres to the settings

Tieske · 2026-02-27T11:20:49Z

src/terminal/text/width.lua

-  end
+local sys = require "system"
+local t = require "terminal"
+-- Stored ambiguous width (1 or 2). 


-- global ambiguous width setting (1 or 2)

Tieske · 2026-02-27T11:21:18Z

src/terminal/text/width.lua

+local AMBIGUOUS_WIDTH = 1 -- Default

-  return width
+--Getter and setter for ambiguous width, in case users want to manage it themselves or check it after calibration.


the doc comments will not properly render in the documentation

Yeah I will do the required change that is true. I still tried catching it with the git diff but missed it I am truly sorry for this

Tieske · 2026-02-27T11:21:40Z

src/terminal/text/width.lua

+function M.get_ambiguous_width()
+    return AMBIGUOUS_WIDTH
+end
+-- Manually sets the ambiguous width setting.


vertical whitespace isn't aligned with the rest of this code file

Tieske · 2026-02-27T11:40:31Z

src/terminal/init.lua

  -- @return true
  -- @within Initialization
-  function M.initialize(opts)
+function M.initialize(opts)


another unnecessary whitespace change. These changes convolute the diff that I'm looking at when reviewing, please don't do this. Also, in this case it even breaks the styling/indentation

I am sorry for this I will improve it

Tieske · 2026-02-27T11:41:40Z

src/terminal/output.lua

 end

-
+function M.isatty()


doc comments are missing, this will not show up in the documentation, but it should

Tieske · 2026-02-27T11:46:25Z

ARCHITECTURE.md

-  - **`terminal.text.width.test(str)`** – writes characters invisibly, measures cursor movement, and records each character’s width. Call during startup or when you first display unknown glyphs.
-  - **`terminal.preload_widths(str)`** – convenience that tests the library’s own box-drawing and progress characters plus any optional `str`. Call once after `terminal.initialize` if you use `terminal.draw` or `terminal.progress`.
- Use **`terminal.size()`** to get terminal dimensions (rows × columns) so you can fit text to the visible area.
+  - **`utf8cwidth(char[, ambiguous_width])`** – returns the display width in columns of a single UTF-8 character (string or codepoint).


this is incorrect, the optional parameter doesn't exist

Tieske · 2026-02-27T11:47:40Z

ARCHITECTURE.md

+
+Ambiguous-width characters default to a width of 1 column. A different
+width (1 or 2) can be specified explicitly via the optional
+`ambiguous_width` parameter.


wrong, this should probably mention the calibration option, or the setter that explicitly sets the value

Tieske · 2026-02-27T11:49:38Z

CHANGELOG.md

  `test` and `test_write` are preserved for API compatibility.
+- feat(progress): account for ANSI escape sequences in sprite width math.
+  Spinner frames and done sprites can now include color/style sequences
+  without breaking cursor rewind behavior.


no need to add a change entry, there is no release yet

Gitkbc mentioned this pull request Feb 24, 2026

fix: update display with handling #204

Open

feat(progress): strip ANSI sequences for sprite width calculations

8ce5d76

Tieske requested changes Feb 24, 2026

View reviewed changes

Gitkbc added 2 commits February 27, 2026 01:49

Refactor width handling: remove caches and use single ambiguous-width…

57588d8

… calibration (LuaSystem >= 0.7.0)

Restore terminal.utils export to public API

50c239d

Tieske requested changes Feb 27, 2026

View reviewed changes



		-- returns a string with all box_fmt characters, to pre-load the character width cache
		-- returns a string with all box_fmt characters



		local function detect_ambiguous_width()
		local row, col = t.cursor.position.get()

Conversation

Gitkbc commented Feb 24, 2026

Uh oh!

Tieske left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gitkbc commented Feb 24, 2026

Uh oh!

Gitkbc commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tieske left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gitkbc Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Gitkbc commented Feb 26, 2026 •

edited

Loading

Gitkbc Feb 27, 2026 •

edited

Loading